fix(db): bound runtime Postgres statements and lock waits - #4613
Conversation
Set a 30-second statement timeout and 5-second lock timeout on writer, reader, audit, and search pool connections. Verify the effective session settings alongside the existing writer guard. Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
Co-authored-by: Jordan Mecom <jm@squareup.com> Signed-off-by: Jordan Mecom <jm@squareup.com>
Schema migrations now run on a connection with both limits lifted. An index build on a populated table, or an ACCESS EXCLUSIVE wait behind live traffic, routinely outlasts the runtime caps, and startup treats a migration failure as fatal — so inheriting them turned a slow migration into a relay that cannot boot. sqlx also takes its migration advisory lock as one waiting statement, so a second replica rolling out would be canceled mid-wait instead of queueing. The connection is closed rather than returned to the pool, since its session still carries no limits. The two values move to DbConfig, wired to BUZZ_DB_STATEMENT_TIMEOUT and BUZZ_DB_LOCK_TIMEOUT with the previous constants as defaults, so a backfill or an incident does not need a code change. A malformed value falls back to the default with a warning: handing it to Postgres would fail every after_connect and take all database access with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Eli Foster <efoster@squareup.com>
Split the exemption into `lift_runtime_timeouts` and `retire_connection` so both halves are observable, and cover them: the migrator's connection reports both limits as `0`, and a single-slot pool hands out a freshly configured connection afterwards rather than the relaxed session. Both assertions were checked against a neutered implementation — a no-op lift and a `drop` instead of a close each fail the test — because a timing-based test does not discriminate here: on an empty database every migration statement finishes well inside the runtime cap, so it would pass with or without the exemption. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Eli Foster <efoster@squareup.com>
elifoster-block
left a comment
There was a problem hiding this comment.
Excluded migrations from the timeouts and set the timeouts as a DBconfig option.
wesbillman
left a comment
There was a problem hiding this comment.
Reviewing on Wes's behalf.
Blocking regression: the migration exemption starts too late. run_migrations calls reject_legacy_nip_rs_cardinality_ambiguity(pool) before run_migrator_without_runtime_timeouts acquires and relaxes its connection. On a pre-v7 database, that preflight executes a potentially large scan of events (including per-row JSON expansion), so it inherits the new 30s statement_timeout. If it exceeds the cap, startup treats the error as fatal and the relay cannot boot—the exact regression the migration exemption is intended to prevent.
Please acquire the dedicated migration connection and lift both limits before running the legacy preflight, then run the migrator on that same connection and retire it on every normal success/error path. The preflight can still remain before sqlx begins its migration transaction. Add a Postgres-backed regression test that makes the preflight exceed a tight runtime timeout while confirming it completes on the exempt connection; the current fresh-database migration test returns before this scan and cannot catch this ordering bug.
Non-blocking hardening note: the relaxed PoolConnection is returned to the runtime pool if the migration future is cancelled after lift_runtime_timeouts but before explicit retirement. The current relay startup awaits migration directly and normally exits on shutdown, which narrows the practical exposure, but making the exemption structurally cancellation-safe would better preserve the stated invariant that an unlimited session can never serve runtime traffic.
This change applies a
statement_timeoutandlock_timeoutwhenever the writer, lazy replica reader, audit, and search pools establish a connection. The limits prevent slow statements and lock waits from holding pool capacity indefinitely. Legitimate work exceeding those bounds will now be canceled and must be retried or redesigned.The bounds are runtime-only. Schema migrations run on a connection with both limits lifted, and that connection is closed rather than returned to the pool: an index build on a populated table, or an
ACCESS EXCLUSIVEwait behind live traffic, routinely outlasts them, and startup treats a migration failure as fatal. sqlx also takes its migration advisory lock as a single waiting statement, so a second replica rolling out would be canceled mid-wait instead of queueing behind the first.lock_timeoutbounds heavyweight and row lock waits. Advisory-lock waits — event replacement, the per-community audit lock, sqlx migrations — are bounded bystatement_timeoutinstead.Both values are configurable through
DbConfig, wired toBUZZ_DB_STATEMENT_TIMEOUTandBUZZ_DB_LOCK_TIMEOUT, defaulting to 30s and 5s. An operator running a backfill or working an incident does not need a code change, and0disables a limit. A malformed value falls back to the default with a warning rather than failing config: handing it to Postgres would fail everyafter_connectand take all database access with it.Testing
cargo test -p buzz-db --libat6f8e115: 94 passed, 153 ignored (Postgres)cargo test -p buzz-relay --libat6f8e115: 830 passed, 10 failed — every failure is pre-existing and needs a live Postgres/Redis, verified by diffing the failure set against the same run with these changes stashedcargo clippy -p buzz-db -p buzz-relay --all-targets -- -D warningsandcargo fmt -- --check: cleangit diff --check origin/main...codex/security-postgres-timeoutsorigin/mainat5c98932Unit tests cover the timeout parser (Postgres spellings accepted; empty, unit-less junk, and
30 secondsrejected in favor of the default) and the env plumbing end to end throughConfig::from_env.Postgres-backed tests, run locally against
postgres:17-alpine:armed_pool_rejects_old_channel_inserts_through_public_api—SHOW statement_timeout/SHOW lock_timeoutare the configured values on both the writer and reader poolsmigration_connection_is_unbounded_and_is_retired_not_reused— the migrator's connection reports0for both, and a single-slot pool hands out a freshly configured connection afterwardsmigrations_ignore_runtime_timeouts_and_leak_no_relaxed_session— a realdb.migrate()against a tight-timeoutDbsucceeds, and every connection in the pool still carries the configured limitsThe exemption assertions were checked against a neutered implementation — a no-op lift and a
dropinstead of a close each fail the test. A timing-based test does not discriminate here: on an empty database every migration statement finishes well inside the runtime cap, so it passes with or without the exemption.Still unverified: these three are
#[ignore = "requires Postgres"]and CI's ignored-test jobs select by explicit filter, so they run only when someone runs them by hand. The full--ignoredbuzz-db suite was not run to completion locally (it exceeds a 10-minute budget); the three above were run individually.Originating Buzz thread:
buzz://message?channel=3928fe05-df61-4b5d-b9c7-d623b9b10ea1&id=3c6c02312f763fbe0d2bfc33a6c1a362f91d0354f3d18b039cf7a0558c1439d1